Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serialize Service Mgmt API's report endpoint body correctly #3294

Merged

Conversation

mayorova
Copy link
Contributor

@mayorova mayorova commented Mar 31, 2023

This fixes the serialization issue with the report endpoint of the Service Management API, initially reported in this comment: #3103 (comment) (item 2).

Actually, it serializes differently in different cases:

  • just using the single transaction element with the value from example:
    'service_token=token&service_id=123&transactions[user_key]=example&transactions[usage][hits]=1'
    
  • the default element plus another element:
    'service_token=token&service_id=123&transactions[user_key]=example&transactions[usage][hits]=1&transactions=%7B%0A%20%20%22user_key%22%3A%20%22example%22%2C%0A%20%20%22usage%22%3A%20%7B%0A%20%20%20%20%22hits%22%3A%201%0A%20%20%7D%0A%7D'
    
  • with manual changes in both elements:
    'service_token=token&service_id=123&transactions=%7B%0A%20%20%22user_key%22%3A%20%22example1%22%2C%0A%20%20%22usage%22%3A%20%7B%0A%20%20%20%20%22hits%22%3A%201%0A%20%20%7D%0A%7D&transactions=%7B%0A%20%20%22user_key%22%3A%20%22example2%22%2C%0A%20%20%22usage%22%3A%20%7B%0A%20%20%20%20%22hits%22%3A%201%0A%20%20%7D%0A%7D'
    

In order to fix this I considered several options:

  1. Do it in requestInterceptor
    This was a bit messy, because at this point the request body is already serialized, and it would be tricky to parse, especially given that sometimes it's serialized as JSON, and other times as form-encoded string, and also could be mixed.

  2. I also tried to write a plugin in the following way:

const ExecuteRequestPlugin: SwaggerUIPlugin = () => {
  return {
    statePlugins: {
      spec: {
        wrapActions: {
          executeRequest: (oriAction: (req: RequestData) => unknown) => (req: RequestData) => {
            // modify req.requestBody
            return oriAction(req)
          }
        }
      }
    }
  }
}

At first it looked promising, as console.log(req) was printing out the requestBody in the browser console, however I then realized that at this point in the execution requestBody is actually not yet available, and console.log was misleading.

  1. So, I went with the approach of overriding the fn.execute function of the SwaggerUI, which in its turn is just imported from swagger-js library.

It's not ideal and quite "hacky", because it tweaks only the specific endpoint and in a very specific way.
Ideally, we could implement something more generic, that would serialize any object with config deepObject and explode in a way that we expect. Maybe, this would also potentially allow us to get rid of this hack of adding [] to the parameters names, like here:

"allowed_service_ids[]": {
"type": "array",
"description": "IDs of the services that need to be enabled, URL-encoded array. To disable all services, set the value to '[]'. To enable all services, add parameter 'allowed_service_ids[]' with no value to the 'curl' command (can't be done in ActiveDocs)",
"items": {
"type": "integer"
}
},
"allowed_sections[]": {

But I would leave this task for later, as an improvement.

For now, the serialization works as expected, the same way as in the original Swagger v1, with a slight difference on special characters encoding (e.g. spaces are encoded as %20 rather than +. But I think it applies to all APIs in general, and should be OK.

Example:

Before

curl -v  -X POST \
  "https://su1.3scale.net/transactions.xml" \
  -d 'service_token=token&service_id=123&transactions%5B0%5D%5Buser_key%5D=key1&transactions%5B0%5D%5Btimestamp%5D=2011-12-30+22%3A15%3A31+%2B08%3A00&transactions%5B0%5D%5Busage%5D%5Bmetric1-1%5D=11&transactions%5B0%5D%5Busage%5D%5Bmetric1-2%5D=12&transactions%5B1%5D%5Buser_key%5D=key2&transactions%5B1%5D%5Btimestamp%5D=2011-12-30+22%3A15%3A31+-08%3A00&transactions%5B1%5D%5Busage%5D%5Bmetric2-1%5D=21&transactions%5B1%5D%5Busage%5D%5Bmetric2-2%5D=22'

decoded body:

'service_token=token&service_id=123&transactions[0][user_key]=key1&transactions[0][timestamp]=2011-12-30+22%3A15%3A31+%2B08%3A00&transactions[0][usage][metric1-1]=11&transactions[0][usage][metric1-2]=12&transactions[1][user_key]=key2&transactions[1][timestamp]=2011-12-30+22%3A15%3A31+-08%3A00&transactions[1][usage][metric2-1]=21&transactions[1][usage][metric2-2]=22'

After

curl -X 'POST' \
  'http://localhost:3001/transactions.xml' \
  -H 'accept: */*' \
  -H 'Content-Type: application/x-www-form-urlencoded' \
  -d 'service_token=token&service_id=123&transactions%5B0%5D%5Buser_key%5D=key1&transactions%5B0%5D%5Btimestamp%5D=2011-12-30%2022%3A15%3A31%20%2B08%3A00&transactions%5B0%5D%5Busage%5D%5Bmetric1-1%5D=11&transactions%5B0%5D%5Busage%5D%5Bmetric1-2%5D=12&transactions%5B1%5D%5Buser_key%5D=key2&transactions%5B1%5D%5Btimestamp%5D=2011-12-30%2022%3A15%3A31%20-08%3A00&transactions%5B1%5D%5Busage%5D%5Bmetric2-1%5D=21&transactions%5B1%5D%5Busage%5D%5Bmetric2-2%5D=22'

decoded body:

'service_token=token&service_id=123&transactions[0][user_key]=key1&transactions[0][timestamp]=2011-12-30 22%3A15%3A31 %2B08%3A00&transactions[0][usage][metric1-1]=11&transactions[0][usage][metric1-2]=12&transactions[1][user_key]=key2&transactions[1][timestamp]=2011-12-30 22%3A15%3A31 -08%3A00&transactions[1][usage][metric2-1]=21&transactions[1][usage][metric2-2]=22'

Before and after are exactly the same, except for the timestamp value:

2011-12-30+22%3A15%3A31+%2B08%3A00
vs
2011-12-30%2022%3A15%3A31%20%2B08%3A00

Need to check if the new encoding actually works with apisonator, it should as %20 is standard.

The above bodies as screenshots in the UI:

Before
transactions-swagger1

After (it's missing a , after the timestamp field, but I don't have time to re-screenshot now)
transactions-openapi3


Docs on SwaggerUI plugins: https://github.com/swagger-api/swagger-ui/blob/master/docs/customization/plugin-api.md

@mayorova mayorova force-pushed the fix-report-body-serialization branch 4 times, most recently from db28217 to 1f102b8 Compare March 31, 2023 14:45
Comment on lines 50 to 60
const buildFormData = (formData: Record<string, boolean | number | string>, data: BodyValue, parentKey?: string) => {
if (data && typeof data === 'object') {
Object.keys(data).forEach((key: string) => {
buildFormData(formData, data[key], parentKey ? `${parentKey}[${key}]` : key)
})
} else {
if (parentKey) {
formData[parentKey] = data ? data : ''
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several issues with this method:
I always find it very confusing two mix up declaration order, object comes as an argument and it's should be the focus of this function but instead is used at the very end for the first time. buildFormData is complex enough to be declared elsewhere, in fact it can be extracted without refactoring since it does not depend on objectToFormData.

The fact that an empty object is being populated from scratch suggests to me a reduce could be used here, ideally:

formData = body.reduce(buildFormData, {})

Recurrence is nice provided the method signature doesn't change but here both data and parentKey generate control couples which IMHO means it should be split (at least) in two. On top of that formData is empty during the first iteration colliding with the actual method signature:

Record<string, never>
// vs
Record<string, boolean | number | string>

There is not one test for this method. I've spent a fair amount of time looking at it and still cannot get the grasp of it so it will be hard to maintain. Considering its purpose is pretty straightforward I strongly suggest it be refactored. I'm working on an alternative right now, I really rather sacrifice verbosity for readability.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think buildFormData is OK to be inside objectToFormData. Otherwise the only thing objectToFormData would do is to call buildFormData, no need to create a new function for that. In any case, what I would do is remove objectToFormData and call buildFormData({}, body) directly from transformReportRequestBody.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that an empty object is being populated from scratch suggests to me a reduce could be used here

That's what I thought initially, but the problem is that we do need to traverse the whole object tree, so I am not sure that reduce would be that helpful (or easy to implement).

I'll address the rest of the comments later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always find it very confusing two mix up declaration order, object comes as an argument and it's should be the focus of this function but instead is used at the very end for the first time.

Well, the only reason why it is used in the end is because buildFormData is declared inside. I can extract it from there, no problem. It's just that I think thought of objectToFormData as a function that does everything, I think it's pretty useless if we just keep the last 3 lines (basically, I agree with what @jlledom says, except I don't think we need to call buildFormData({}, body) directly from transformReportRequestBody.
Anyway, I will see if I can make it more readable.

buildFormData is complex enough to be declared elsewhere, in fact it can be extracted without refactoring since it does not depend on objectToFormData.

But do we need to have an exported function somewhere else (where?) if we know it is not going to be used by anything rather than this function? To me it makes more sense to keep the things together in this case, this will avoid the need to jump around multiple files/lines in the same file to understand what the function does.

On top of that formData is empty during the first iteration colliding with the actual method signature: [...]

Doesn't Record<string, boolean | number | string> accept {} ?

There is not one test for this method.

Fair enough, although the test would be for objectToFormData as it's the one that is exported.

I've spent a fair amount of time looking at it and still cannot get the grasp of it so it will be hard to maintain.

What's confusing about it? What would you suggest?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to give it a spin, but I did not manage to get rid of recursion in favor of reduce and friends.

I don't think it makes sense to waste more time on this. It's a very specific case, and it currently works for the issue it is meant to solve.

If you have an alternative implementation - please share 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't insist, I lost this battle. One last war cry in favor of SRP though: a function that does everything is wrong. A function must do one and only one thing. And it's OK to have a 1 or 2 line function as long as it complies with SRP. It makes it more readable and more maintainable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL, "function that does everything" was probably an overstatement 😅 IMO objectToFormData actually does 1 thing (transform one object format to another), but it's self-contained, so you don't need to jump around the code to understand what it does.

But well, I understand your point also. Does this look better to you? a7a2e45

app/javascript/src/ActiveDocs/ThreeScaleApiDocs.ts Outdated Show resolved Hide resolved
app/javascript/src/ActiveDocs/ThreeScaleApiDocs.ts Outdated Show resolved Hide resolved
app/javascript/src/ActiveDocs/ThreeScaleApiDocs.ts Outdated Show resolved Hide resolved
app/javascript/src/Types/SwaggerTypes.ts Outdated Show resolved Hide resolved
app/javascript/src/Types/swagger.d.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
Copy link
Contributor

@jlledom jlledom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the code is complicated, the first thing I did is try to make this work locally. I arranged everything (apisonator, etc) and tried.

It works fine and the backend registers the metrics, but I found a bug: it seems the number of hits registered by the backend is greater than I specified in the request. Please check the attached video, observe how the Hits metric is 25 and after sending hits: 5 then Hits is increased to 36, it should be 25+5=30, which leads me to think it's adding up also the 6 I set for to the hits.3 metric.

Screencast.from.2023-04-04.14-43-04.webm

app/javascript/src/Types/swagger.d.ts Outdated Show resolved Hide resolved
app/javascript/src/ActiveDocs/ThreeScaleApiDocs.ts Outdated Show resolved Hide resolved
app/javascript/src/ActiveDocs/ThreeScaleApiDocs.ts Outdated Show resolved Hide resolved
Comment on lines 50 to 60
const buildFormData = (formData: Record<string, boolean | number | string>, data: BodyValue, parentKey?: string) => {
if (data && typeof data === 'object') {
Object.keys(data).forEach((key: string) => {
buildFormData(formData, data[key], parentKey ? `${parentKey}[${key}]` : key)
})
} else {
if (parentKey) {
formData[parentKey] = data ? data : ''
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think buildFormData is OK to be inside objectToFormData. Otherwise the only thing objectToFormData would do is to call buildFormData, no need to create a new function for that. In any case, what I would do is remove objectToFormData and call buildFormData({}, body) directly from transformReportRequestBody.

@mayorova mayorova force-pushed the fix-report-body-serialization branch from 5bd2238 to 986609e Compare April 12, 2023 12:19
@mayorova
Copy link
Contributor Author

@jlledom

which leads me to think it's adding up also the 6 I set for to the hits.3 metric.

Yes... that's the case. I don't like this behavior at all, and I raised an issue about that https://issues.redhat.com/browse/THREESCALE-7681
The last comment from Andreu points to another JIRA to make this behavior configurable.

But as of today this behavior is expected.

Thanks for taking time for verifying the end-to-end functionality! 🙌

@mayorova mayorova force-pushed the fix-report-body-serialization branch from fb0f801 to ffa3f34 Compare April 13, 2023 08:31
@mayorova mayorova force-pushed the fix-report-body-serialization branch from 3dfe288 to a7a2e45 Compare April 13, 2023 10:44
@jlledom
Copy link
Contributor

jlledom commented Apr 13, 2023

But as of today this behavior is expected.

Super weird, but OK.

@mayorova mayorova force-pushed the fix-report-body-serialization branch from 4b6c286 to 1f56654 Compare April 13, 2023 15:59
@mayorova mayorova merged commit 78503bd into THREESCALE-3927-update-to-openapi Apr 13, 2023
@mayorova mayorova deleted the fix-report-body-serialization branch April 13, 2023 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants